fix(form): stabilize ProForm/BaseForm renders and FormItem memoization#9605
Conversation
…rativeHandle - ProForm: memoize default contentRender with useCallback to avoid new function identity every render and unnecessary BaseForm content remounts - BaseForm: fix nested JSDoc on formatValues; useImperativeHandle deps use formRef instead of formRef.current; content useMemo depends on formRef - FormItem: useDeepCompareMemo depends on child fieldProps (not duplicate omit() in deps array) for correct deep-compare memoization Co-authored-by: 陈帅 <wasd2144@hotmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough三个表单相关文件经过内存化优化：BaseForm 调整了内容渲染和指令句柄的依赖追踪；FormItem 简化了字段属性提取逻辑；ProForm 将内联的内容渲染函数改为 useCallback 包装。 Changes表单内存化与依赖优化
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 48 seconds.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates JSDoc comments, refactors dependency arrays in BaseForm and FormItem, and memoizes the contentRender function in ProForm. The review feedback points out a performance regression in FormItem due to improper dependency tracking in useDeepCompareMemo and identifies an unnecessary inclusion of a stable ref object in a useMemo dependency array in BaseForm.
| ['onBlur', 'onChange'], | ||
| ), | ||
| ], | ||
| [childFieldProps], |
There was a problem hiding this comment.
The change to the dependency array of omitOnBlurAndOnChangeProps introduces a performance regression. By using [childFieldProps] instead of the omitted object, the useDeepCompareMemo will now trigger a re-computation whenever onBlur or onChange change (which are often unstable inline functions in React forms). The previous implementation correctly ignored these event handlers by omitting them before the deep comparison, ensuring that only changes to other field props would trigger a re-render of the child component.
| [childFieldProps], | |
| [omit(childFieldProps || {}, ['onBlur', 'onChange'])], |
| } | ||
| return wrapItems; | ||
| }, [grid, RowWrapper, items, contentRender, submitterNode]); | ||
| }, [grid, RowWrapper, items, contentRender, submitterNode, formRef]); |
There was a problem hiding this comment.
Adding formRef to the dependency array of useMemo is unnecessary and potentially misleading. formRef is a ref object created via useRef within the component, so its reference is stable and will not trigger a re-computation of the memoized value. Furthermore, adding a ref object to dependencies does not solve "stale snapshot" issues because useMemo does not track changes to the .current property. If the intent is to ensure the latest form instance is used, accessing formRef.current inside the factory is sufficient, but it won't trigger updates if the ref is updated after the initial render.
| }, [grid, RowWrapper, items, contentRender, submitterNode, formRef]); | |
| }, [grid, RowWrapper, items, contentRender, submitterNode]); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9605 +/- ##
==========================================
- Coverage 88.53% 88.48% -0.06%
==========================================
Files 368 368
Lines 11257 11259 +2
Branches 4157 4156 -1
==========================================
- Hits 9966 9962 -4
- Misses 1139 1144 +5
- Partials 152 153 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:M
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing unnecessary re-renders/remounts in the form system by stabilizing render callback references and tightening React hook dependency usage, plus a small JSDoc fix for better generated docs/IDE hints.
Changes:
- Stabilize
ProForm’s defaultcontentRenderby switching from an inline function to a memoized callback. - Adjust
BaseFormmemoization/dependency handling aroundcontentanduseImperativeHandleto avoid ref.currentin dependency lists. - Fix a broken nested JSDoc block for
validateFieldsReturnFormatValuedocumentation, and refactorFormItemmemo dependencies.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/form/layouts/ProForm/index.tsx | Makes default contentRender a stable callback to reduce remounts. |
| src/form/components/FormItem/index.tsx | Refactors memoization inputs for omitted fieldProps. |
| src/form/BaseForm/BaseForm.tsx | Fixes JSDoc and adjusts hook deps to avoid using ref.current in deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const childFieldProps = React.isValidElement(filedChildren) | ||
| ? (filedChildren.props as Record<string, any>)?.fieldProps | ||
| : undefined; | ||
|
|
||
| const omitOnBlurAndOnChangeProps = useDeepCompareMemo( | ||
| () => | ||
| omit( | ||
| // @ts-ignore | ||
| filedChildren?.props?.fieldProps || {}, | ||
| ['onBlur', 'onChange'], | ||
| ), | ||
| [ | ||
| omit( | ||
| // @ts-ignore | ||
| filedChildren?.props?.fieldProps || {}, | ||
| childFieldProps || {}, | ||
| ['onBlur', 'onChange'], | ||
| ), | ||
| ], | ||
| [childFieldProps], | ||
| ); |
Summary
Form module review (performance, API shape, tests) and targeted fixes.
Performance
contentRenderwas an inline function, so it was a new reference every parent re-render. That causedBaseForm’scontentuseMemoto recompute and remount the main form body unnecessarily. The default render is now a stableuseCallback.content’suseMemonow listsformRef(the ref object) instead of effectively depending on a staleformRef.currentsnapshot.useImperativeHandlefor the extendedformRefAPI no longer usesformRef.currentin the dependency array (refs should not be read for dependency lists).API / docs
formatValues.validateFieldsReturnFormatValueinBaseForm(stray/**broke the block comment and hurt generated docs/IDE hints).Tests
pnpm run tscandpnpm exec vitest run tests/form(326 tests) pass.Summary by CodeRabbit
发版说明